Skip to content

Conversation

@subrata-ms
Copy link
Contributor

@subrata-ms subrata-ms commented Dec 4, 2025

Work Item / Issue Reference

AB#40714

GitHub Issue: #341


Summary

Problem: Segmentation fault occurs during Python's garbage collection when freeing ODBC handles.

Stack Trace Analysis:

0-2: Signal handler (SIGSEGV)
3: libmsodbcsql-18.5.so.1.1 - CRASH LOCATION
4: SQLFreeHandle() from ODBC driver
5: SqlHandle::free() from ddbc_bindings
6-11: pybind11 call stack
12: Python method call
20: slot_tp_finalize during del
21-27: Python GC finalizing objects

Root Cause: The crash occurs when;

  1. Python's garbage collector runs during finalization
  2. Objects with [del] methods are being cleaned up
  3. The [del] calls [close()] which internally calls SQLFreeHandle()
  4. The ODBC driver crashes because:
  • Handle is already freed (double-free)
    
  • Handle is in invalid state
    
  • Connection handle freed before cursor statement handle
    
  • Wrong finalization order due to circular references
    

Fix Details:
This pull request introduces a critical fix to the handle cleanup logic in the SQL bindings for Python, specifically addressing potential segfaults during interpreter shutdown. The change ensures that both statement and database connection handles are not freed if Python is shutting down, preventing invalid memory access.

Handle cleanup logic improvements:

  • Updated the SqlHandle::free() method in mssql_python/pybind/ddbc_bindings.cpp to skip freeing both statement (SQL_HANDLE_STMT) and database connection (SQL_HANDLE_DBC) handles during Python shutdown, rather than only statement handles. This prevents segfaults caused by freeing handles in the wrong order when their parent resources may have already been released.

Copilot AI review requested due to automatic review settings December 4, 2025 11:57
@github-actions github-actions bot added the pr-size: large Substantial code update label Dec 4, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes a critical segmentation fault issue that occurs during Python interpreter shutdown when freeing ODBC handles. The fix prevents double-free errors and invalid memory access by skipping handle cleanup for both STMT (Type 3) and DBC (Type 2) handles during Python shutdown, as their parent handles may already be destructed.

Key Changes:

  • Extended the shutdown protection in SqlHandle::free() to skip freeing both DBC and STMT handles during Python shutdown (previously only STMT handles were protected)
  • Added comprehensive test suite with 13 test cases covering various shutdown scenarios including aggressive segfault reproduction, GC finalization order, exception handling, and circular references

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
mssql_python/pybind/ddbc_bindings.cpp Updated SqlHandle::free() to skip freeing both Type 2 (DBC) and Type 3 (STMT) handles during Python shutdown, preventing segfaults from accessing already-freed parent handles
tests/test_013_SqlHandle_free_shutdown.py Added comprehensive test suite with 13 test cases using subprocess isolation to verify handle cleanup behavior during Python shutdown across all handle types (ENV, DBC, STMT)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

📊 Code Coverage Report

🔥 Diff Coverage

55%


🎯 Overall Coverage

74%


📈 Total Lines Covered: 5265 out of 7029
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/init.py (50.0%): Missing lines 92-93,95-96,98,100-101,104,108,110
  • mssql_python/connection.py (66.7%): Missing lines 321,326
  • mssql_python/pybind/ddbc_bindings.cpp (100%)

Summary

  • Total: 27 lines
  • Missing: 12 lines
  • Coverage: 55%

mssql_python/init.py

Lines 88-114

   88     This prevents resource leaks during interpreter shutdown by ensuring
   89     all ODBC handles are freed in the correct order before Python finalizes.
   90     """
   91     # Make a copy of the connections to avoid modification during iteration
!  92     with _connections_lock:
!  93         connections_to_close = list(_active_connections)
   94 
!  95     for conn in connections_to_close:
!  96         try:
   97             # Check if connection is still valid and not closed
!  98             if hasattr(conn, "_closed") and not conn._closed:
   99                 # Close will handle both cursors and the connection
! 100                 conn.close()
! 101         except Exception as e:
  102             # Log errors during shutdown cleanup for debugging
  103             # We're prioritizing crash prevention over error propagation
! 104             try:
  105                 driver_logger.error(
  106                     f"Error during connection cleanup at shutdown: {type(e).__name__}: {e}"
  107                 )
! 108             except Exception:
  109                 # If logging fails during shutdown, silently ignore
! 110                 pass
  111 
  112 
  113 # Register cleanup function to run before Python exits
  114 atexit.register(_cleanup_connections)

mssql_python/connection.py

Lines 317-330

  317         # This ensures ODBC handles are freed in correct order, preventing leaks
  318         try:
  319             if hasattr(mssql_python, "_register_connection"):
  320                 mssql_python._register_connection(self)
! 321         except AttributeError as e:
  322             # If registration fails, continue - cleanup will still happen via __del__
  323             logger.warning(
  324                 f"Failed to register connection for shutdown cleanup: {type(e).__name__}: {e}"
  325             )
! 326         except Exception as e:
  327             # Catch any other unexpected errors during registration
  328             logger.error(
  329                 f"Unexpected error during connection registration: {type(e).__name__}: {e}"
  330             )


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.cpp: 66.2%
mssql_python.row.py: 66.2%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 83.5%
mssql_python.cursor.py: 84.3%
mssql_python.__init__.py: 84.9%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Copy link
Contributor

@sumitmsft sumitmsft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have put some comments to it - for the change in behavior.

@bewithgaurav since you had worked on this change earlier, please review this PR.

Copy link
Collaborator

@bewithgaurav bewithgaurav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments requesting changes
restructuring, adding tests covering multithreaded scenarios and some cleanup needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: large Substantial code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants